Skip to content

Conversation

CoveMB
Copy link
Contributor

@CoveMB CoveMB commented Sep 19, 2025

Fixes #662

Summary

  • Fixes Remix URL code parameter encoding to be reliably decodable, including both Unicode and special characters.
  • Adds focused unit tests for remixURL behavior and wires UI tests into CI.

Motivation

  • Previous encoding could be brittle across environments when decoding via atob (decodeURIComponent(...)). This ensures consistent, binary-safe Base64 and trims padding to keep the URL compact.

Key Changes

  • packages/ui/src/solidity/remix.ts: Update encoding to Base64 over UTF-8 bytes and store once as encodedCode; keep padding trimmed.
  • packages/ui/src/solidity/remix.node.test.ts: Add Node tests verifying:
    • code param decodes back to original source.
    • deployProxy flag behavior.
  • packages/ui/package.json: Add test script using Node’s test runner with tsx.
  • package.json: Add test:ui script and tsx dev dependency.
  • .github/workflows/test.yml: Add dedicated ui job to run UI tests.
  • yarn.lock: Update due to new dev dependency.

Behavioral Impact

  • User-facing: Only affects the generated Remix URL’s code parameter; behavior is otherwise unchanged.
  • Backward compatibility: No breaking changes expected.

How To Test
Before Remix side MR is merged

  • create any contract
    • Open in remix, it should decode
      After Remix side MR is merged
  • In ui make a Solidity Account contract with modules AND use a special character a contract name
    • Open in remix, it should decode

** Note**

@CoveMB CoveMB marked this pull request as ready for review September 19, 2025 01:40
@CoveMB CoveMB requested review from a team as code owners September 19, 2025 01:40
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
package.json (1)

32-32: tsx only at root couples UI tests to hoisting

UI package executes node --import tsx but doesn’t declare tsx itself. Add tsx to packages/ui devDependencies to avoid breakage under non-hoisted installs or PnP.

Apply in packages/ui/package.json:

   "devDependencies": {
+    "tsx": "^4.19.2",
     "@rollup/plugin-alias": "^5.0.0",
packages/ui/package.json (1)

12-13: Expose a top-level test:ui in this workspace (optional) and declare tsx locally

Scripts look good. Consider also adding tsx to this package’s devDependencies (see root comment) to decouple from hoisting.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0f075b6 and 6933f06.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (5)
  • .github/workflows/test.yml (1 hunks)
  • package.json (3 hunks)
  • packages/ui/package.json (2 hunks)
  • packages/ui/src/solidity/remix.node.test.ts (1 hunks)
  • packages/ui/src/solidity/remix.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ui/src/solidity/remix.node.test.ts (1)
packages/ui/src/solidity/remix.ts (1)
  • remixURL (1-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: check
  • GitHub Check: build (stylus, default)
  • GitHub Check: build (cairo, default)
  • GitHub Check: build (stellar, compile)
  • GitHub Check: build (stellar, default)
  • GitHub Check: build (solidity, default)
  • GitHub Check: mcp
  • GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (5)
package.json (1)

10-10: LGTM: added test:ui script

This nicely scopes UI tests and aligns with the new CI job.

.github/workflows/test.yml (1)

43-52: Make Node version explicit for the UI job

node --import tsx --test depends on recent Node. Either ensure your setup action pins Node ≥18 (preferably 20+) or add an explicit actions/setup-node step here for determinism.

If setup already pins Node ≥20, ignore.

packages/ui/src/solidity/remix.node.test.ts (3)

12-21: LGTM: round‑trip sanity check

Covers the basic path and asserts presence of the code param.


23-31: LGTM: deployProxy flag behavior

Correct: only set when upgradeable is true.


33-96: LGTM: complex/Unicode contract case

Good coverage for SPDX, imports, inheritance, and Unicode literals (e.g., emoji).

Copy link

socket-security bot commented Sep 19, 2025

Caution

Review the following alerts detected in dependencies.

According to your organization's Security Policy, you must resolve all "Block" alerts before proceeding. Learn more about Socket for GitHub.

Action Severity Alert  (click "▶" to expand/collapse)
Block Low
[email protected] is a AI-detected potential code anomaly.

Notes: No definitive malware detected in this fragment. The main security concern is supply-chain risk from dynamically loading plugins from potentially untrusted sources. To mitigate, enforce strict plugin provenance, disable remote plugin loading, verify plugin integrity, and apply least-privilege execution for plugins.

Confidence: 1.00

Severity: 0.60

From: package.jsonnpm/[email protected]

ℹ Read more on: This package | This alert | What is an AI-detected potential code anomaly?

Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at [email protected].

Suggestion: An AI system found a low-risk anomaly in this package. It may still be fine to use, but you should check that it is safe before proceeding.

Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore npm/[email protected]. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

Block Low
[email protected] is a AI-detected potential code anomaly.

Notes: The code is a conventional, well-scoped implementation of an RxJS-like concat operator. No malicious behavior, data exfiltration, or suspicious I/O detected in this fragment. Security risk is low; malware likelihood is negligible for this isolated operator function.

Confidence: 1.00

Severity: 0.60

From: yarn.locknpm/[email protected]

ℹ Read more on: This package | This alert | What is an AI-detected potential code anomaly?

Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at [email protected].

Suggestion: An AI system found a low-risk anomaly in this package. It may still be fine to use, but you should check that it is safe before proceeding.

Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore npm/[email protected]. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

Block Low
[email protected] is a AI-detected potential code anomaly.

Notes: No explicit network exfiltration, reverse shell, or credential theft is present in this fragment. However, the code assembles and compiles arbitrary code via the Function constructor and invokes passed-in functions immediately (twice). That behavior constitutes a strong dangerous primitive (arbitrary code execution) which can be abused if any inputs (strings or args) are attacker-controlled. Treat this module as risky in threat models where inputs are not fully trusted; review call sites and sanitize/validate inputs or avoid dynamic evaluation.

Confidence: 1.00

Severity: 0.60

From: ?npm/@modelcontextprotocol/[email protected]npm/[email protected]

ℹ Read more on: This package | This alert | What is an AI-detected potential code anomaly?

Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at [email protected].

Suggestion: An AI system found a low-risk anomaly in this package. It may still be fine to use, but you should check that it is safe before proceeding.

Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore npm/[email protected]. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

View full report

@CoveMB
Copy link
Contributor Author

CoveMB commented Sep 24, 2025

@coderabbitai review

Copy link
Contributor

coderabbitai bot commented Sep 24, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@CoveMB CoveMB enabled auto-merge (squash) September 24, 2025 20:55
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
packages/ui/ts-node-register.cjs (1)

6-9: Optional: speed up tests with swc transpiler

You can shave test time by enabling ts-node’s swc transpiler (keep transpileOnly). Requires adding @swc/core as a devDependency.

Apply this diff (and add @swc/core devDependency):

 require('ts-node').register({
   transpileOnly: true,
   project: path.join(__dirname, 'tsconfig.test.json'),
+  transpiler: 'ts-node/transpilers/swc-experimental',
 });
packages/ui/src/solidity/remix.test.ts (1)

5-9: Make decoder tolerant of trimmed padding

Since URLs trim “=”, some atob implementations require re-padding. Re-pad defensively to avoid environment-specific flakiness while still mirroring Remix’s flow.

Apply this diff:

-const decodeBase64 = (b64Payload: string) => {
-  const raw = atob(decodeURIComponent(b64Payload));
+const decodeBase64 = (b64Payload: string) => {
+  const decoded = decodeURIComponent(b64Payload);
+  const pad = (4 - (decoded.length % 4)) % 4;
+  const padded = pad ? decoded + '='.repeat(pad) : decoded;
+  const raw = atob(padded);
   const bytes = Uint8Array.from(raw, c => c.charCodeAt(0));
   return new TextDecoder().decode(bytes);
 };
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6933f06 and 28afbb6.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (7)
  • package.json (2 hunks)
  • packages/ui/ava.config.js (1 hunks)
  • packages/ui/package.json (3 hunks)
  • packages/ui/src/solidity/remix.test.ts (1 hunks)
  • packages/ui/src/solidity/remix.ts (1 hunks)
  • packages/ui/ts-node-register.cjs (1 hunks)
  • packages/ui/tsconfig.test.json (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • packages/ui/tsconfig.test.json
  • packages/ui/ava.config.js
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/ui/package.json
  • package.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T19:26:06.112Z
Learnt from: ericglau
PR: OpenZeppelin/contracts-wizard#652
File: packages/core/confidential/print-versioned.js:1-1
Timestamp: 2025-09-18T19:26:06.112Z
Learning: CJS shims like `module.exports = require('./dist/print-versioned')` in packages/core/confidential are expected to be CJS-only and are used specifically within packages/ui, not as general-purpose exports requiring conditional exports mapping.

Applied to files:

  • packages/ui/ts-node-register.cjs
🪛 GitHub Check: format-lint
packages/ui/src/solidity/remix.ts

[failure] 10-10:
Unexpected any. Specify a different type


[failure] 7-7:
Unexpected any. Specify a different type

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build (solidity, default)
🔇 Additional comments (2)
packages/ui/src/solidity/remix.ts (2)

16-16: LGTM: using URLSearchParams for code param

This ensures proper percent-encoding of reserved characters.


4-14: Fix unbounded spread and remove any casts; resolve lint failures and avoid stack/heap issues

  • String.fromCharCode(...bytes) on the full Uint8Array can overflow the call stack for large sources.
  • ESLint “Unexpected any” on (globalThis as any) is failing format-lint.
  • Use chunked conversion and typed access to global features. This keeps your cross-runtime intent, preserves “trim padding,” and makes lint green.

Apply this diff:

-  // Encode to base64 in a way that works in both browser and Node.
-  const encodedCode = ((): string => {
-    // Prefer browser btoa when available
-    if (typeof (globalThis as any).btoa === 'function') {
-      const bytes = new TextEncoder().encode(code);
-      const binary = String.fromCharCode(...bytes);
-      return (globalThis as any).btoa(binary).replace(/=*$/, '');
-    }
-    // Fallback to Node Buffer
-    return Buffer.from(code, 'utf8').toString('base64').replace(/=*$/, '');
-  })();
+  // Cross‑runtime Base64 over UTF‑8 bytes. Trims "=" padding to keep URLs compact.
+  const encodedCode = ((): string => {
+    const g = globalThis as unknown as {
+      btoa?: (data: string) => string;
+      Buffer?: { from(input: string, encoding: string): { toString(encoding: 'base64'): string } };
+    };
+    // Prefer Node/Buffer when available (works in Node and many bundler environments)
+    if (g.Buffer && typeof g.Buffer.from === 'function') {
+      return g.Buffer.from(code, 'utf8').toString('base64').replace(/=*$/, '');
+    }
+    // Browser path: build the binary string in chunks to avoid call‑stack overflow
+    if (g.btoa && typeof TextEncoder !== 'undefined') {
+      const bytes = new TextEncoder().encode(code);
+      let binary = '';
+      const CHUNK = 0x8000; // 32k chunks
+      for (let i = 0; i < bytes.length; i += CHUNK) {
+        binary += String.fromCharCode(...bytes.subarray(i, i + CHUNK));
+      }
+      return g.btoa(binary).replace(/=*$/, '');
+    }
+    throw new Error('No Base64 encoder available in this environment');
+  })();

@ericglau
Copy link
Member

@SocketSecurity ignore npm/[email protected]

Copy link

@zeljkoX zeljkoX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@CoveMB CoveMB merged commit f1c7c6d into OpenZeppelin:master Oct 8, 2025
17 of 19 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Open in Remix has encoding error for some contracts
3 participants